Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the camera capture layout into an adaptive 7-mode orchestrator that dispatches across dedicated posture layouts (CompactPortraitLayout, CompactShortLayout, FoldableLayout, TabletPortraitLayout, and TabletLandscapeLayout) to support various device geometries, and adds comprehensive Robolectric tests. The review feedback suggests utilizing the existing CameraLayoutMode enum instead of raw string literals for active layout names, passing isErrorDialogShowing explicitly from CaptureScaffold for more robust state-based detection, and using placeRelative instead of place in custom layouts to ensure proper Right-to-Left (RTL) support.
I am having trouble creating individual review comments. Click here to see my feedback.
ui/components/capture/src/main/java/com/google/jetpackcamera/ui/components/capture/CompactPortraitLayout.kt (104-105)
Using raw String literals (like "CompactPortraitLayout") for identifying active layouts is prone to typos and makes refactoring difficult. Since the codebase already has a well-defined CameraLayoutMode enum, and LocalCameraLayoutMode is already provided as a CompositionLocal, we can use it directly to determine if a compact layout is active (i.e., when the mode is COMPACT or COMPACT_SLIM). This eliminates the need for LocalActiveCaptureLayout entirely. If LocalActiveCaptureLayout must be kept for other reasons, it should at least be marked as internal to adhere to the visibility guidelines in the style guide.
References
- Proper Visibility Modifiers: Ensure all new functions, properties, and classes use the most restrictive visibility modifier possible (e.g., private, internal) while still allowing necessary access. Avoid public visibility unless explicitly required for external API exposure. (link)
ui/components/capture/src/main/java/com/google/jetpackcamera/ui/components/capture/CompactPortraitLayout.kt (335-336)
Currently, isErrorDialogShowing is passed as a parameter to CompactPortraitLayout but is not supplied by CaptureScaffold when it invokes CaptureScaffoldLayout. As a result, isErrorDialogShowing will always default to false in production, making the layout entirely reliant on the onSizeChanged modifier of the errorDialog slot to detect if the error dialog is active. To make this state-based detection more robust and less reliant on layout-pass measurements, consider updating CaptureScaffold to pass isErrorDialogShowing = cameraErrorState.value is CameraErrorUiState.Showing to CaptureScaffoldLayout.
ui/components/capture/src/main/java/com/google/jetpackcamera/ui/components/capture/CompactShortLayout.kt (345-348)
In custom layouts, it is idiomatic to use placeRelative instead of place to automatically support Right-to-Left (RTL) layouts correctly. Consider replacing place with placeRelative here to ensure consistency with other custom layouts in the codebase (such as AdaptivePortraitCaptureLayout).
placeable.placeRelative(
x = (resolvedWidth - placeable.width) / 2,
y = (resolvedHeight - placeable.height) / 2
)
Summary
Introduces the 7-mode
CameraLayoutModeclassifier with14.dp(±7.dp) hysteresis deadbands and turnsCaptureScaffoldLayoutinto a livemovableContentOf-backed adaptive orchestrator dispatching across all 5 posture layouts introduced inkim/adaptive/layouts. Wires realCaptureScaffoldUI controls across compact, foldable, tablet, and compact-short postures viaCaptureSlotScope.orientationandisCompact, isolating temporary vertical placeholders strictly toTopBar(#649) andCaptureModeToggle(#635).Key Changes
CameraLayoutMode.kt(new):CameraLayoutModeenum (COMPACT,COMPACT_SLIM,COMPACT_SHORT,COMPACT_SHORT_SLIM,FOLDABLE,TABLET_PORTRAIT,TABLET_LANDSCAPE),LocalCameraLayoutMode,classifyCameraLayoutMode,isFullScreenCompactPhoneWindow,rememberIsFullScreenCompactPhone, andrememberCameraLayoutMode.CaptureLayout.kt:CaptureScaffoldLayoutinto the 7-modeBoxWithConstraintsorchestrator, wrappingviewfinderinmovableContentOfso the live preview surface reparents across posture transitions without disposal.COMPACT/COMPACT_SLIM→CompactPortraitLayout(withincludeStatusBarsInTopBar = !isFullScreenCompactPhone),COMPACT_SHORT/COMPACT_SHORT_SLIM→CompactShortLayout,FOLDABLE→FoldableLayout,TABLET_PORTRAIT→TabletPortraitLayout, andTABLET_LANDSCAPE→TabletLandscapeLayout.@Previewfunctions covering all 7CameraLayoutModepostures across portrait and horizontal orientations.CaptureScaffold.kt&CompactShortLayout.kt:CaptureSlotScope.orientationandisCompactintoCaptureScaffoldDefaults.ZoomControls(enabling real verticalZoomBarinStartSideRail),CaptureScaffoldDefaults.TopBar(vertical placeholder for#649), andCaptureScaffoldDefaults.CaptureModeToggle(interactive verticalRole.Switchplaceholder for#635with mode switching and disabled-rationale snackbars), and passesisErrorDialogShowingexplicitly toCaptureScaffoldLayout.CaptureScaffoldDefaults.QuickSettingsto renderQuickSettingsScaffoldContentonCompactPortraitLayoutandQuickSettingsContent(without bottom-sheet nav-bar insets) inside floating Quick Settings cards on side-rail layouts, and usesplaceRelativeinsideCompactShortLayout'sConstrainedSlot.SideRailCaptureLayouts.kt:topOverlayClearancepadding to the bottom-centercaptureModeCarouselslot whenconfig.clearRailsForQuickSettingsistrueso full-width carousels never overlapStartSideRailorEndSideRail.Technical Notes
14.dp(±7.dp) Hysteresis Deadbands &SideEffectHistory:classifyCameraLayoutModebiases every threshold crossing (360.dpslim,480.dpshort,600.dpwide,480.dpwide-height,960.dp/1.45ffoldable-vs-tablet, and1:1square tablet diagonal) by±7.dparoundpreviousMode.rememberCameraLayoutModestorespreviousModein a plain holder mutated insideSideEffectso committed transitions never trigger a redundant second recomposition and aborted compositions never corrupt hysteresis state.FOLDABLErequireswidthDp >= 600.dp,heightDp >= 480.dp,maxDim < 960.dp, andmaxDim < minDim × 1.45f. UsingminDim/maxDimkeeps squarish unfolded inner displays (673×841dpto852×883dp) inFoldableLayout(0.50frails) across90°rotation, while routing tall compact tablets (600×900dp, ratio1.50) toTabletPortraitLayout(0.70flower thumb zone) and large 4:3 tablets (1024×768dp,maxDim >= 960.dp) to tablet layouts.rememberIsFullScreenCompactPhoneobservesLocalConfigurationandComponentActivity.addOnMultiWindowModeChangedListener(sw in 1 until 600, non-multi-window,maxDim >= 480.dp, container within2.dpof screen bounds) so rotating a wide compact phone never jumps intoTabletLandscapeLayout, while360×360dpsquare cover screens (maxDim < 480.dp) still route toCompactShortLayout.movableContentOfAcross Subcomposition: WrappingviewfinderinmovableContentOfpreserves its slot table andDisposableEffectlifecycle (composeCount == 1,disposeCount == 0) even when moving betweenCompactPortraitLayout(BottomSheetScaffoldSubcomposeLayout),CompactShortLayout(BoxWithConstraints), and standardBoxside-rail layouts.Pre-Existing Bugs Fixed
CaptureLayout.kt): Replaced hardcodedincludeStatusBarsInTopBar = falsefromkim/adaptive/layoutswith!isFullScreenCompactPhoneso visible multi-window status bars on narrow compact panes (480×800dp) no longer overlap the top status indicator row.CaptureModeCarouselcorner overlap (SideRailCaptureLayouts.kt): Added horizontaltopOverlayClearancepadding toslots.captureModeCarouselon rail-cleared postures (FoldableLayout,TabletPortraitLayout,TabletLandscapeLayout) while leavingCompactShortLayout(clearRailsForQuickSettings = false) singly constrained bywithCondensedSizeCaps.CaptureScaffold.kt): GatedQuickSettingsScaffoldContent(WindowInsets.navigationBarsIgnoringVisibility) onLocalActiveCaptureLayout.current == "CompactPortraitLayout", renderingQuickSettingsContentdirectly inside side-rail floating cards.Not Included Yet
TopBar(#649), VerticalCaptureModeToggle(#635), &QuickSettingsOverlayCard(#648): Vertical side-rail slots temporarily renderPlaceholderControlBoxstand-ins (with full toggle & snackbar behavior onCaptureModeToggle), and side-rail Quick Settings renders realQuickSettingsContentinsideQuickSettingsOverlayCardPlaceholderuntil#648merges.kim/adaptive/layout-rotation): Anchors the portrait control stack to the physical side edge, hosts the viewfinder in an un-rotatedOuterViewfinderHostsoSurfaceViewand 10-bit HDR stay active, and adds the layout mode to the debug readout.kim/adaptive/viewfinder-rotation): UpdatesPreview.targetRotationin place viaTransientSessionSettings.windowRotationwhenever the host window rotates.kim/adaptive/content-gravity): Handles per-component rotation. Tooltip (#622) and zoom bar RTL polish (#644) remain independent.Testing
CameraLayoutModeTest(10 JUnit4 tests): Initial 7-mode classification, rotation-pair invariants,isFullScreenCompactPhoneWindowedge cases (2.dptolerance, multi-window,sw600dp,360×360dpcover display), and bidirectional14.dphysteresis sweeps across all 6 boundaries.AdaptiveCaptureLayoutTest(12 Robolectric Compose UI tests): Device-qualifier routing across all 7 modes, multi-windowstatusBarstop-bar clearance, end-to-endCaptureScaffoldon1280×800dpTABLET_LANDSCAPE(real shutter/flip/carousel, verticalZoomBar, clickable vertical mode-toggle placeholder, single-node Quick Settings card), and 10-step dynamic resize across all 7 modes verifyingcomposeCount == 1anddisposeCount == 0.